asymmetric laplace distribution added #4392

chandan5362 · 2020-12-29T12:52:36Z

Asymmetric Laplace distribution added for #4351
please do let me know if there are any unnecessary changes or any changes that are yet to be made.

codecov · 2020-12-29T13:22:56Z

Codecov Report

Merging #4392 (d333141) into master (aabec02) will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #4392      +/-   ##
==========================================
+ Coverage   88.15%   88.17%   +0.01%     
==========================================
  Files          88       88              
  Lines       14564    14587      +23     
==========================================
+ Hits        12839    12862      +23     
  Misses       1725     1725

Impacted Files	Coverage Δ
pymc3/distributions/__init__.py	`100.00% <ø> (ø)`
pymc3/distributions/continuous.py	`94.59% <100.00%> (+0.14%)`	⬆️

ricardoV94

I added a few comments. I think you also need tests for the random method in test_distributions_random.py

pymc3/distributions/continuous.py

pymc3/tests/test_distributions.py

ricardoV94 · 2020-12-29T15:09:12Z

Great work @chandan5362!

One other minor thing you should do once you fix everything is to run pre-commit to auto-format your code to be in line with the rest of the PyMC3 codebase. You have some information here, in case this is new for you. That addresses the test that is currently failing.

chandan5362 · 2020-12-30T21:19:49Z

Hey @ricardoV94 , I have been trying to add unit test for random module for laplace_asymmetric but I ended up comparing numpy variable with theano.tensor varibale. which is giving following error.:unamused:
TypeError: Variables do not support boolean operations.
Here is the code that I have been trying to add

  def test_laplace_asymmetric(self):
          def ref_rand(size,kappa,b):
              u = np.random.uniform(size=size)
              x = -np.log((1 - u) * (1 + kappa ** 2)) / (kappa * b) * (
                  u > ((kappa ** 2) / (1 + kappa ** 2))
              ) + kappa * np.log(
                  u * (1 + kappa ** 2) / (kappa ** 2)
              ) / b * (
                  u < ((kappa ** 2) / (1 + kappa ** 2))
              )
  
              return x
  
          pymc3_random(pm.AsymmetricLaplace,{"b":Domain([0,1,np.inf]),"kappa":Rplus}, ref_rand=ref_rand)

would you please help me here a little ?

Sayam753

This looks great so far @chandan5362. Coming to the error message, you can try out the comments below and check if the test passes.
The pymc3_random function checks if samples from distribution's random method and ref_rand come from same distribution. We need to have tests to check for shape consistencies as well. You can add a test for this very similar to how Normal distribution is tested here.

pymc3/distributions/continuous.py

ricardoV94 · 2020-12-31T16:32:58Z

Also, you might have already done this, but if not, you should also implement the BaseTest. Here is the code for the Laplace distribution:

https://github.com/pymc-devs/pymc3/blob/master/pymc3/tests/test_distributions_random.py#L373-L376

Something similar should work for your distribution. I think this takes care of testing if random works with different sizes/shapes

Edit: This was probably what @Sayam753 was saying. Sorry for the repetition :)

chandan5362 · 2020-12-31T17:36:29Z

Thanks a lot @Sayam753 @ricardoV94 , It was really helpful.
Now that I know what actually was happening, I edited the random function and as Sayam said, values were supposed to be drawn with respect to the model, I did the same and the test is running perfectly fine.

twiecki · 2020-12-31T17:55:12Z

pymc3/tests/test_distributions.py

@@ -218,7 +220,12 @@ def build_model(distfam, valuedomain, vardomains, extra_args=None):
        distfam("value", shape=valuedomain.shape, transform=None, **vals)
    return m

-
+def laplace_asymmetric_logpdf(value, symmetry,scale = None):


Is this taken from scipy? If so I would mention that in the doc-string and also add a TODO that it should be removed once scipy adds it.

Is this taken from scipy?
yeah @twiecki , It has been taken from SciPy.

OK cool, then just add those notes.

Also not properly black formatted.

hey @twiecki ,
Wish you a very very Happy New Year.
please do review the PR and let me know if anything is yet to be added or removed.

pymc3/distributions/continuous.py

Sayam753

I addressed some changes to be made. I need some time to review the maths behind random method. Meanwhile you can work on the suggestions.

One more thing, you can give a mention of AsymmetricLaplace distribution in api source continuous.rst. Then we can have this distribution to show up in docs.
There also needs to be mention of Support, Mean and Variance in docstring for consistency.

Also, scipy 1.6.0 has been released a few days ago. It contains the asymmetric laplace distribution. In order to use it in test cases, we have to upgrade PyMC3 requirements. Ping @MarcoGorelli to ask if upgrading scipy will break anything or not.

pymc3/distributions/continuous.py

Sayam753 · 2021-01-03T14:49:47Z

pymc3/distributions/continuous.py

+    See also: https://en.wikipedia.org/wiki/Asymmetric_Laplace_distribution
+    """
+
+    def __init__(self, b, kappa, testval=0.0, *args, **kwargs):


PyMC3 base Distribution class already handles testval that can be passed through kwargs. So, I think testval should not be there.
While building str representation of a distribution/model, PyMC3 inspects all arguments passed in __init__ method. With this way, testval may come up in the representation and can certainly be confusing to someone not familiar with test values.

Okay, Thanks
I will have a look at it.

Once I remove the testval , unit test outputs an error. I think, testval should be there. Please have a look at it.

Does something like this at the end of __init__ fix it?

self.mode = mu

Assuming you are already using mu (and have converted it to a tensor_variable)

Interesting. I am curious to know how testval interacts self.mode.

I think it uses the mode to define the testval. If the mean or median is provided, one of those is used instead.

pymc3/distributions/continuous.py

pymc3/tests/test_distributions.py

pymc3/distributions/continuous.py

Sayam753

This is turning out to be in good shape. The random, logp method and their tests need to be updated to also account for mu parameter.

pymc3/distributions/continuous.py

MarcoGorelli · 2021-01-05T12:12:43Z

In order to use it in test cases, we have to upgrade PyMC3 requirements. Ping @MarcoGorelli to ask if upgrading scipy will break anything or not.

My suggestion would be to first merge #4374, so that we test on the oldest supported scipy version, and then skip any tests which use a too modern scipy (see that same PR for an example) and run those tests during, say, the arviz-compat job, which uses the latest available versions of dependencies

I don't think the requirements should be updated if a feature from scipy is only needed to run the tests

Sayam753 · 2021-01-05T13:57:49Z

Last step would be to add this distribution for docs in api source continuous.rst and a mention in RELEASE-NOTES.md

Sayam753 · 2021-01-05T15:45:42Z

Great work @chandan5362 🎉 . Thanks @twiecki @ricardoV94 for the helpful review.

twiecki · 2021-01-05T17:36:10Z

What a great example of a high-quality PR and review! 💪

chandan5362 · 2021-01-05T17:56:41Z

Thanks a lot @ricardoV94 @twiecki @Sayam753 for being helpful and supportive .

ricardoV94 reviewed Dec 29, 2020

View reviewed changes

pymc3/distributions/continuous.py Show resolved Hide resolved

pymc3/tests/test_distributions.py Outdated Show resolved Hide resolved

Sayam753 requested changes Dec 31, 2020

View reviewed changes

pymc3/distributions/continuous.py Outdated Show resolved Hide resolved

pymc3/distributions/continuous.py Outdated Show resolved Hide resolved

pymc3/distributions/continuous.py Outdated Show resolved Hide resolved

twiecki reviewed Dec 31, 2020

View reviewed changes

ricardoV94 reviewed Jan 1, 2021

View reviewed changes

pymc3/distributions/continuous.py Outdated Show resolved Hide resolved

ricardoV94 requested changes Jan 2, 2021

View reviewed changes

pymc3/distributions/continuous.py Outdated Show resolved Hide resolved

twiecki assigned Sayam753 Jan 2, 2021

Sayam753 requested changes Jan 3, 2021

View reviewed changes

Sayam753 reviewed Jan 4, 2021

View reviewed changes

pymc3/distributions/continuous.py Outdated Show resolved Hide resolved

Sayam753 requested changes Jan 5, 2021

View reviewed changes

pymc3/distributions/continuous.py Outdated Show resolved Hide resolved

pymc3/distributions/continuous.py Show resolved Hide resolved

chandan5362 added 5 commits January 5, 2021 18:53

asymmetric laplace distribution added

8b6b15a

unit test for random function added

b9c335e

continuous.py updated

a6cc0ba

suggested changes added

8b65c66

updated

d333141

chandan5362 added 2 commits January 5, 2021 19:52

RELEASE-NOTES.md and continuous.rst updated

a19c8f9

39146c7

Sayam753 approved these changes Jan 5, 2021

View reviewed changes

Sayam753 merged commit 044c407 into pymc-devs:master Jan 5, 2021

Sayam753 mentioned this pull request Jan 5, 2021

Add asymetric laplace distribution #4351

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

asymmetric laplace distribution added #4392

asymmetric laplace distribution added #4392

chandan5362 commented Dec 29, 2020

codecov bot commented Dec 29, 2020 •

edited

Loading

ricardoV94 left a comment

ricardoV94 commented Dec 29, 2020

chandan5362 commented Dec 30, 2020

Sayam753 left a comment

ricardoV94 commented Dec 31, 2020 •

edited

Loading

chandan5362 commented Dec 31, 2020

twiecki Dec 31, 2020

chandan5362 Dec 31, 2020

twiecki Dec 31, 2020

twiecki Dec 31, 2020

chandan5362 Dec 31, 2020

Sayam753 left a comment •

edited

Loading

Sayam753 Jan 3, 2021

chandan5362 Jan 3, 2021

chandan5362 Jan 4, 2021

ricardoV94 Jan 4, 2021

Sayam753 Jan 4, 2021

ricardoV94 Jan 4, 2021 •

edited

Loading

Sayam753 left a comment

MarcoGorelli commented Jan 5, 2021 •

edited

Loading

Sayam753 commented Jan 5, 2021

Sayam753 commented Jan 5, 2021

twiecki commented Jan 5, 2021

chandan5362 commented Jan 5, 2021

asymmetric laplace distribution added #4392

asymmetric laplace distribution added #4392

Conversation

chandan5362 commented Dec 29, 2020

codecov bot commented Dec 29, 2020 • edited Loading

Codecov Report

ricardoV94 left a comment

Choose a reason for hiding this comment

ricardoV94 commented Dec 29, 2020

chandan5362 commented Dec 30, 2020

Sayam753 left a comment

Choose a reason for hiding this comment

ricardoV94 commented Dec 31, 2020 • edited Loading

chandan5362 commented Dec 31, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sayam753 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ricardoV94 Jan 4, 2021 • edited Loading

Choose a reason for hiding this comment

Sayam753 left a comment

Choose a reason for hiding this comment

MarcoGorelli commented Jan 5, 2021 • edited Loading

Sayam753 commented Jan 5, 2021

Sayam753 commented Jan 5, 2021

twiecki commented Jan 5, 2021

chandan5362 commented Jan 5, 2021

codecov bot commented Dec 29, 2020 •

edited

Loading

ricardoV94 commented Dec 31, 2020 •

edited

Loading

Sayam753 left a comment •

edited

Loading

ricardoV94 Jan 4, 2021 •

edited

Loading

MarcoGorelli commented Jan 5, 2021 •

edited

Loading